-
Notifications
You must be signed in to change notification settings - Fork 2.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow up to maxRedirects upon receiving HTTP 301 status #2939
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2939 +/- ##
=======================================
Coverage 98.17% 98.17%
=======================================
Files 143 143
Lines 12602 12602
=======================================
Hits 12372 12372
Misses 156 156
Partials 74 74
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you, @k0ral !
LGTM.
Awaiting second LGTM+Approval from any other contributor to this repo before merging.
Thank you, @valbeat ! |
I know this is already merged, but I would like to suggest that since we are doing a breaking change for these methods we consider updating them to accept an options struct instead of just the maxRedirects option. This will allow future options to be added without a breaking change. If they previously used an options struct this wouldn't need to be a breaking change. We can't avoid a break now, but we can avoid future breaks. |
Good idea, @WillAbides - do you want to make a PR that addresses this and we will hold off on the next release until it is merged? |
@WillAbides - I was requested to cut a new release, but just remembered that you wanted to change this Do you want me to hold off on cutting the new release, or shall we simply go ahead and break it twice? I'm leaning toward breaking it twice at the moment, since I like to be responsive when users want a new release and it has been about a month since the last one. |
I'll go ahead on the release. We can work on an improved interface for the next breaking release. |
@gmlewis I agree with going ahead with the release because the need to move to options is hypothetical at this point. |
* fix(deps): update module github.com/google/go-github/v55 to v56 * fix(google/go-github): address the breaking change of go-github - google/go-github#2939 - #2340 (comment) --------- Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Shunsuke Suzuki <[email protected]> Co-authored-by: Shunsuke Suzuki <[email protected]>
The
roundTripWithOptionalFollowRedirect()
method currently takes a boolean parameterfollowRedirects
, used to support HTTP 301 redirections. However, at most 1 redirection is handled at the moment (the parameter is set tofalse
after the first iteration.)I've stumbled upon multiple repositories where
https://api.github.com/repos/<OWNER>/<PROJECT>/branches/<BRANCH>
redirects twice ; as a result, theGetBranch()
method only follows the first redirection, then returns the 2nd HTTP 301 response.This MR proposes to generalize the
followRedirects
boolean into amaxRedirects
integer, to support an arbitrary number of redirections. This impacts multiple methods, not justGetBranch()
.